-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(releases): adding correct confirmation dialog to archive release flow #7827
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
No changes to documentation |
Component Testing Report Updated Nov 18, 2024 4:28 PM (UTC) ✅ All Tests Passed -- expand for details
|
⚡️ Editor Performance ReportUpdated Mon, 18 Nov 2024 16:29:53 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
…/sanity into corel-255-archive-confirmation
@@ -0,0 +1,17 @@ | |||
import {type ReleaseDocument} from '../store/types' | |||
|
|||
export const activeScheduledRelease: ReleaseDocument = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking that we'd continue to create others here eg activeASAPRelease
as are necessary for test cases across our suite. All fixtures would be correct and valid states, eg activeASAPRelease
would not have an intendedPublishAt
since that is not a valid state machine value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😚 👌
vi.mock('sanity', () => ({ | ||
SANITY_VERSION: '0.0.0', | ||
useTranslation: vi.fn().mockReturnValue({t: vi.fn()}), | ||
vi.mock('../../../../store/useReleaseOperations', () => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried in vain to simplify the way this could be mocked, but a mixture of issues an nuance with how vi.mock
is hoisted and executed mean we still need these mock statements explicitly in our tests rather than in generalised mock files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice nice nice nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rogue thought I had when I was reviewing this related to where wea re placing the fixtures.
I think it might make better sense to keep them somewhere along like we do with the TestProvider in the test
directory?
Right now we are keeping in the core/releases but I feel it will come a point where we might want to use releases in other places for one reason or another. Especially since this does kinda touch more parts than this self contained part (the first thing that comes to mind right now are some components in structure
).
I'm not married to the idea but I think findability might be better :)
</Text> | ||
), | ||
}) | ||
console.error(archivingError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rogue console log or meant to be here? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed similar approach to what we do in Publish and schedule, so this is intended
@@ -67,6 +150,8 @@ export const ReleaseMenuButton = ({disabled, release}: ReleaseMenuButtonProps) = | |||
{!release?.state || release.state === 'archived' ? ( | |||
<MenuItem | |||
onClick={handleUnarchive} | |||
// TODO: disabled as CL action not yet impl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a linear story for this? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we do corel-256
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments and a suggestion, great job :)
packages/sanity/src/core/releases/store/createReleaseOperationStore.ts
Outdated
Show resolved
Hide resolved
telemetry.log(UnarchivedRelease) | ||
setIsPerformingOperation(false) | ||
// noop | ||
// TODO: similar to handleArchive - complete once server action exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for deleting this? Do we expect unarchive to change significantly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will change from what it currently is in corel
(will be updated to to be similar to archive). I considered implementing it, but feel it's better to complete the full task once the action can be test alongside. There's a separate task I'm monitoring and will complete once ready
packages/sanity/src/core/releases/tool/components/ReleaseMenuButton/ReleaseMenuButton.tsx
Show resolved
Hide resolved
|
||
import {type ReleaseOperationsStore} from '../../createReleaseOperationStore' | ||
|
||
export const useReleaseOperationsMock: Mocked<ReleaseOperationsStore> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a great pattern to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Description
The current flow for archiving, although correctly using the server action, was not implementing the correct UX.
Now, archiving brings up the correct confirmation dialog.
Unarchiving has also been no-op-ed as the action doesn't exist right now
What to review
Specifically, looking for thoughts on:
useReleaseOperations
- https://github.com/sanity-io/sanity/pull/7827/files#diff-4aa171c7928a5161b60dde60a11cd53762cc08c16e27a114558f6da103cacb03ReleaseMenuButton.test
suite - https://github.com/sanity-io/sanity/pull/7827/files#diff-49a11b1a7f4b6c7f99fadf3b5161e16b71d6aea1af3afe1d1e5d25a58513aa54Interested to know thoughts both on directory structure for these mocks and fixtures, and the naming convention for these files also. Further thoughts:
vi.mock
usage for export mocks) and fixtures (objects and other primitives that can be used consistently across all our tests)mocks
sit alongside the files they are mocking the shape of; fixtures sit inside the domain space they are related tomocks
can still be overridden on test by test to customiseTesting
Note testing thoughts above
Notes for release
N/A